Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

code optimisation for circular ref issue #62

Closed
wants to merge 6 commits into from

Conversation

SB-rohitdesai
Copy link

Mock server not working In case of circular references.

Motivation and Context

Description

Getting 500 Error or Sometimes might need to wait a long time before getting this error when spec having circular references

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

Copy link

@rainum rainum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SB-rohitdesai, it looks good! Do you plan to add any tests for this functionality though?

@SB-rohitdesai
Copy link
Author

@SB-rohitdesai, it looks good! Do you plan to add any tests for this functionality though?

Actually everything is already covered to test async await functionality.

@coveralls
Copy link

coveralls commented Jun 13, 2024

Pull Request Test Coverage Report for Build 9494945514

Details

  • 7 of 7 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.05%) to 93.753%

Totals Coverage Status
Change from base Build 8325139558: 0.05%
Covered Lines: 1181
Relevant Lines: 1236

💛 - Coveralls

lib/dereference.js Outdated Show resolved Hide resolved
Copy link

@bhaskarsontakke bhaskarsontakke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes looks good to me. Though few comments are added

lib/dereference.js Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Jun 13, 2024

Pull Request Test Coverage Report for Build 9498353515

Details

  • 7 of 7 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.05%) to 93.753%

Totals Coverage Status
Change from base Build 8325139558: 0.05%
Covered Lines: 1181
Relevant Lines: 1236

💛 - Coveralls

@SB-rohitdesai SB-rohitdesai requested a review from rainum June 13, 2024 11:43
Copy link

@rainum rainum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All those functions look synchronous to me 🤔 Why did you make them async? Can we get some benchmarking results on how it improves the derefencing speed?

@SB-rohitdesai
Copy link
Author

All those functions look synchronous to me 🤔 Why did you make them async? Can we get some benchmarking results on how it improves the derefencing speed?

Hi @rainum
As I updated all three files (In all three PR's) , we are able to reduce the time , if you want to see that , you can check the video which I attached to the ticket

@SB-rohitdesai
Copy link
Author

All those functions look synchronous to me 🤔 Why did you make them async? Can we get some benchmarking results on how it improves the derefencing speed?

Hi @rainum As I updated all three files (In all three PR's) , we are able to reduce the time , if you want to see that , you can check the video which I attached to the ticket

Hi @rainum May be because of async its faster , But its giving response quickly than previous code

@rainum
Copy link

rainum commented Jun 21, 2024

All those functions look synchronous to me 🤔 Why did you make them async? Can we get some benchmarking results on how it improves the derefencing speed?

Hi @rainum As I updated all three files (In all three PR's) , we are able to reduce the time , if you want to see that , you can check the video which I attached to the ticket

Hi @rainum May be because of async its faster , But its giving response quickly than previous code

We can't solve issues by using an "imperial" approach. There should be a reason for this. I don't want to have so kind of "magical" code in our codebase.

@brendarearden, do you have any thoughts regarding this one?

@brendarearden
Copy link

As far as I know making a function async shouldn't do anything to the execution time of the individual function and it might slightly increase the time because it has to create a promise in addition to running the function. So there is definitely a different underlying reason as to why this change optimized the execution time and I agree that we should have an answer to that. Is it possible that an await was not added somewhere so we just never wait for it to execute? I feel like the linter would've caught that.

@brendarearden
Copy link

I'd recommend using yalc or something similar to pull these changes into Prism, create a draft PR with the yalced version and add tests in the prism draft pr to demonstrate the optimization if you are not able to add test directly in here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants